Skip to content

Various Tests lints #2288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jan 26, 2024
Merged

Various Tests lints #2288

merged 8 commits into from
Jan 26, 2024

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Dec 11, 2023 •
edited
Loading

In advance of switching to testthat 3, I thought I'd revisit the tests a little to make them more consistent.

The rationale behind

-  expect_true(all(st_bbox(p1_norm) == c(0,0,1,1)))
+ expect_equal(unclass(st_bbox(p1_norm)), c(0,0,1,1), check.attributes = FALSE)

is that it accidentally fails for is object has length 0.

See this example

# before, the test passed.
pt <- sf::st_sfc( sf::st_point( x = c(0,1,3,3)))
expect_true( all( attr( pt, "zbox" ) == c(3,3) ) )

# with the refactor, this would fail (that's why I commented it out.
pt <- st_sfc(st_point( x = c(0,1,3,3)))
# expect_equal(attr( pt, "zbox" ), c(3, 3)) # FIXME: now NULL

olivroy
olivroy commented Jan 26, 2024
View reviewed changes
tests/testthat/test_zm_range.R

test_that("sf::st_z_range and sf::st_z_range returns correct value from sfc objects", {
pt <- st_sfc(st_point( x = c(0,1,3,3)))
# expect_equal(attr( pt, "zbox" ), c(3, 3)) # FIXME: now NULL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @edzer , please let me know if you agree with those changes, they are mostly stylistics, but when doing some linting, it helped me uncover this bug (or wrong test). If you want I don't mind breaking it down into smaller PRs.

The main goal is to standardize the tests, reduce noise before switching to testthat 3e.

@edzer edzer merged commit a5f9f0f into r-spatial:main Jan 26, 2024
@edzer
Copy link
Member

edzer commented Jan 26, 2024

Great, thanks!!

@olivroy olivroy deleted the tests branch January 31, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@olivroy @edzer